Use form for expression validation in alert profiles #2667
Conversation
Codecov Report
@@ Coverage Diff @@
## 5.6.x #2667 +/- ##
==========================================
+ Coverage 54.13% 54.31% +0.18%
==========================================
Files 558 558
Lines 40641 40671 +30
==========================================
+ Hits 22000 22090 +90
+ Misses 18641 18581 -60
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
johannaengland
force-pushed
the
working-ip-validation
branch
from
891a3bc to
8ba967b
Compare
johannaengland
changed the title
There was a problem hiding this comment.
I'm glad you're doing this cleanup :)
This isn't a full review (I'm sure @hmpf knows the form stuff better than me), but I'm not sure we've thought of this part: What about existing broken data?
The simple solution would be to just let AlertEngine log its brains out when existing data isn't valid (and pray that someone actually looks at the logs). Or, is there someway more in-your-face way we can inform the user that their profile is broken?
There was a problem hiding this comment.
Tested manually this time, and I notice two issues:
-
If I select "in match" from the dropdown and enter an invalid value, my dropdown selection is reset to "equals match" when the form displays the validation error. Not sure if this problem was there before (I guess there was no validation, so nobody ever noticed). This should be fixed, IMHO.
-
Also, is my value supposed to be blanked out from the form if it was invalid? What if I input a really long string and only had a small typo? Now I have to enter everything all over again, rather than just fix the one typo...
-
I noticed the usage examples quoted on the form page includes CIDR addresses, not just plain IP address. I'm not entirely sure these are supposed to work (but it is more likely than not), you should check how AlertEngine actually parses the values. If they are meant to work, someone has definitely used CIDR values in their profile, and then these must also validate. For this, we have
nav.util.is_valid_cidr(). You would have to use something likeis_valid_ip(ip) or is_valid_cidr(ip)for the form logic...
For 1. and 2. I added a new issue (#2673) since this requires a lot more effort and probably a bigger restructuring of the alert profile views. |
Great!
Agreed - as discussed IRL, these issues are likely not related to this PR at all, just to the fact that Alert Profiles predates Django and there has a very wonky usage of Django functionality. |
Add validation of IP addresses
johannaengland
force-pushed
the
working-ip-validation
branch
from
8929cf3 to
ef27237
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Closes #1876.
Improvement of #2649.
This moves the validation for expressions to django forms and also fixes the existing Expression form to correctly handle both string inputs, in case of IP addresses e.g. and multiple choice inputs.